Skip to content

fix(sponsor-sync): use SELECT result to detect row existence in addSponsorPermission, not UPDATE affected-row count#529

Merged
smarcet merged 1 commit intomainfrom
hotfix/per-sponsor-permission-tracking
Apr 15, 2026
Merged

fix(sponsor-sync): use SELECT result to detect row existence in addSponsorPermission, not UPDATE affected-row count#529
smarcet merged 1 commit intomainfrom
hotfix/per-sponsor-permission-tracking

Conversation

@romanetar
Copy link
Copy Markdown
Collaborator

@romanetar romanetar commented Apr 15, 2026

ref https://app.clickup.com/t/86b94hpua

Summary by CodeRabbit

  • Bug Fixes
    • Improved sponsor permission tracking to correctly handle cases where permissions already exist, ensuring consistent behavior when permissions are re-applied or after retry operations.

…onsorPermission, not UPDATE affected-row count

Signed-off-by: romanetar <roman_ag@hotmail.com>
@romanetar romanetar requested a review from smarcet April 15, 2026 16:50
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 15, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1badf2f9-c4cd-4675-8084-39e61a478fdd

📥 Commits

Reviewing files that changed from the base of the PR and between 6439c0a and 9de7377.

📒 Files selected for processing (2)
  • app/Models/Foundation/Main/Member.php
  • tests/Unit/Entities/SponsorPermissionTrackingTest.php

📝 Walkthrough

Walkthrough

The Member::addSponsorPermission() method was refactored to explicitly check for Sponsor_Users row existence using a locked SELECT query before executing an UPDATE. The return value now indicates row existence rather than relying on affected row counts. Two unit tests validate the updated behavior across different row lifecycle scenarios.

Changes

Cohort / File(s) Summary
Member Model Update
app/Models/Foundation/Main/Member.php
Modified addSponsorPermission() to use locked SELECT 1 ... FOR UPDATE to check row existence before updating. Returns 0 if row absent, 1 if row present. Updated docblock to clarify return semantics.
Permission Tracking Tests
tests/Unit/Entities/SponsorPermissionTrackingTest.php
Added two test methods: one validating return value 1 when permission already present, another validating the retry flow after row deletion and eager recreation via Sponsor::addUser().

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~18 minutes

Possibly related PRs

Suggested reviewers

  • smarcet

Poem

🐰 Check before you hop, dear database friend,
SELECT FOR UPDATE, let locks defend!
Row exists or not, the return now rings true,
Idempotent updates—the right thing to do! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the primary change: updating addSponsorPermission to use SELECT result instead of UPDATE affected-row count for detecting row existence.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch hotfix/per-sponsor-permission-tracking

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-529/

This page is automatically updated on each push to this PR.

Copy link
Copy Markdown
Collaborator

@smarcet smarcet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@smarcet smarcet merged commit f3923fa into main Apr 15, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants